Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add suggestion for nested fields #81480

Merged
merged 1 commit into from
Feb 1, 2021
Merged

Add suggestion for nested fields #81480

merged 1 commit into from
Feb 1, 2021

Conversation

b-naber
Copy link
Contributor

@b-naber b-naber commented Jan 28, 2021

Closes #81220

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 28, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
estebank
estebank reviewed Jan 29, 2021
View reviewed changes
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for tackling this!

compiler/rustc_typeck/src/check/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/expr.rs Outdated Show resolved Hide resolved
src/test/ui/issues/issue-81220.stderr Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/expr.rs Outdated
return Some(field_path_clone);
} else {
field_path_clone.push(ident);
if let Some(path) = self.check_for_nested_field(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I hadn't thought about doing an exhaustive search... We probably want to keep some limits here to avoid accidentally blowing up the compilation time, as this is O(n^2) in the worst case. Maybe up to 4 levels deep and 1000 fields, otherwise early return.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually exponential run-time in the worst case. Even limiting the recursion depth and the number of fields to 4 and 1000, resp. still would result in 1000^4 calls in the worst case. I changed this to 3 and 100. 100 might be too low, alternatively we could maybe track and limit the number of recursive calls instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, 100 should be fine. Keep in mind that the degenerate case requires all levels of nesting to be structs with 100 fields each. A cheeky solution would be to have a global count of fields per level that we pass down (so at any added dot you're always sure you're not going above N total subfields) but that complicates the logic unnecessarily.

compiler/rustc_typeck/src/check/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/expr.rs Outdated Show resolved Hide resolved
src/test/ui/issues/issue-81220.rs Outdated Show resolved Hide resolved
@b-naber
Copy link
Contributor Author

b-naber commented Jan 29, 2021

@estebank Thanks for the review. I fixed the previous commit. Can you take another look, please?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank
Copy link
Contributor

There's a recently added check to keep the number of tests in the issues directory down. Unless we're addressing a very specific ticket, where there's no appropriate description, we want to create tests in an appropriate subdirectory with a name that describes what is being tested. I would move src/test/ui/issues/issue-81220.* to src/test/ui/suggestions/non-existent-field-present-in-subfield.* or something like that.

@estebank
Copy link
Contributor

estebank commented Jan 29, 2021 •
edited
Loading

I just noticed that we don't check for the fields being accessible (privacy), but that is fine. People will try it and get an error telling them that.

I think this PR can be merged as soon as we make CI happy :) Also, if you could squash your commits into a single one, that will make navigating the history much easier. (I know we could squash merge on our side, but we don't because for larger PRs we want to keep that history if it has semantic meaning.)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@b-naber
Copy link
Contributor Author

b-naber commented Jan 29, 2021 •
edited
Loading

@estebank I don't understand why the test fails. I did bless the test.

@estebank
Copy link
Contributor

It seems like you need to --bless again because you --blessed first and then ran rustfmt.

@b-naber
Copy link
Contributor Author

b-naber commented Jan 30, 2021

Mh it has something to do with run-rustfix. If I remove the .fixed file and test (with --bless) without run-rustfix being included in the test then the test passes, but if I include run-rustfix then the test fails, even if I bless again.

@estebank
Copy link
Contributor

https://github.com/rust-lang/rust/pull/81480/files#diff-5dbdd13d22e9b8f4724e3435534639a8047d8714c72bc1fdd6e46eac5b68d8f7 and https://github.com/rust-lang/rust/pull/81480/files#diff-22da95cc8535eeedafa55938fb4e4bd1293014cf79a3ceaa1808c2849e4eaa26 look different. --bless creates .fixed files when needed and updates them. Could it be you generated the .fixed file manually or before changing the formatting?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@b-naber
Copy link
Contributor Author

b-naber commented Jan 30, 2021 •
edited
Loading

@estebank That's not the reason, I pushed the latest changes and it still fails. Does rustfix expect the code to compile after the suggestions have been applied? It indicates that the last example in the test errors (because we test here that the limit in the depth of the recursion works), but maybe the test fails because it expects the code in the test to compile if we use run-rustfix?

@estebank
Copy link
Contributor

Ah! Yes. That's what's happening. The resulting code has to be compilable without errors/warnings. There are two options. If we want to ensure this doesn't regress what I do is split the test into two different files. One that will always be fixable and one that isn't. The other alternative is to just remove the run-rustfix. I prefer the former to avoid regressions.

@b-naber
Copy link
Contributor Author

b-naber commented Jan 30, 2021

@estebank CI passes now. I seperated the tests, but thinking about it, I don't really see the usefulness of the second test if we have no way to fix that no suggestion is included in the stderr output. Do you want me to remove that test again?

@estebank
Copy link
Contributor

@bors r+

@b-naber I think it makes sense to keep it.

@bors
Copy link
Contributor

bors commented Jan 31, 2021

📌 Commit 9946b54 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2021
@jonas-schievink jonas-schievink mentioned this pull request Jan 31, 2021
Closed
This was referenced Jan 31, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 31, 2021
@bors
Auto merge of rust-lang#81596 - jonas-schievink:rollup-utk14gr, r=jon…
941343e 
…as-schievink

Rollup of 11 pull requests

Successful merges:

 - rust-lang#80092 (2229: Fix issues with move closures and mutability)
 - rust-lang#80404 (Remove const_in_array_repeat)
 - rust-lang#81255 (Don't link with --export-dynamic on wasm32-wasi)
 - rust-lang#81480 (Add suggestion for nested fields)
 - rust-lang#81549 (Misc ip documentation fixes)
 - rust-lang#81566 (Add a test for rust-lang#71202)
 - rust-lang#81568 (Fix an old FIXME in redundant paren lint)
 - rust-lang#81571 (Fix typo in E0759)
 - rust-lang#81572 (Edit multiple error code Markdown files)
 - rust-lang#81589 (Fix small typo in string.rs)
 - rust-lang#81590 (Stabilize int_bits_const)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 991b313 into rust-lang:master Feb 1, 2021
@rustbot rustbot added this to the 1.51.0 milestone Feb 1, 2021
@b-naber b-naber deleted the nested_fields_suggestion branch February 1, 2021 10:54
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 1, 2021
Closed
@m-ou-se m-ou-se mentioned this pull request Feb 3, 2021
Merged
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 3, 2021
@m-ou-se
Rollup merge of rust-lang#81716 - m-ou-se:fix-ice, r=eddyb
4617418 
Fix non-existent-field ICE for generic fields.

I mentioned this ICE in a chat and it took about 3 milliseconds before `@eddyb` found the problem and said this change would fix it. :)

This also changes one the field types in the related test to one that triggered the ICE.

Fixes rust-lang#81627.
Fixes rust-lang#81672.
Fixes rust-lang#81709.

Cc rust-lang#81480 `@b-naber` `@estebank.`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank left review comments

Assignees

@estebank estebank

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.51.0
Development

Successfully merging this pull request may close these issues.

When field isn't found, see if any of the available fields has it
6 participants
@b-naber @rust-log-analyzer @estebank @bors @rust-highfive @rustbot